Android: add better nullability checks for nullability annotations added in NDK 26 by finagolfin · Pull Request #4850 · swiftlang/swift-corelibs-foundation

finagolfin · 2023-12-01T14:07:49Z

This is needed because Bionic recently added a bunch of these annotations. I made sure this pull doesn't break anything by testing most of it with the previous NDK 25c also. I used this patch with others to build the Swift toolchain for my Android CI, finagolfin/swift-android-sdk#122, and the Termux app for Android, which now uses NDK 26b.

@drodriguez or @compnerd, please review.

finagolfin · 2023-12-01T14:38:04Z

+                        munmap(buffer, length)
+                    }
+                  #else
                    return NSData.NSDataReadResult(bytes: data!, length: mapSize) { buffer, length in


As shown in my Bionic link above, mmap() now returns _Nonnull, so this force-unwrap fails.

It feels rather unfortunate to ifdef around this here instead of at the place we call mmap.

parkera · 2023-12-01T19:53:14Z

@swift-ci test

parkera · 2023-12-01T19:53:55Z

+                        munmap(buffer, length)
+                    }
+                  #else
                    return NSData.NSDataReadResult(bytes: data!, length: mapSize) { buffer, length in


It feels rather unfortunate to ifdef around this here instead of at the place we call mmap.

parkera · 2023-12-01T19:55:09Z

            do {
                guard fm.fileExists(atPath: _url.path) else { throw _NSErrorWithErrno(ENOENT, reading: true, url: url) }
                _stream = try FileManager.default._fileSystemRepresentation(withPath: _url.path) { fsRep in
+                    #if os(Android)


Similarly here, can we do this by converting the type somehow at the call to fts_open instead of around all uses of the type before that point?

Maybe there is, I just don't know how to do it. This is simply the best I came up with after seeing these Swift type-casting issues for the first time, and I wanted to ask if there was a better way.

@glbrntt, you recently added similar code to NIO, wdyt?

NIO wraps all of the syscalls and does any platform-specific shenanigans within the wrapper.

OK, but that will still require adapting these types for nullability annotations: see my recent patch of your new NIO code for Android.

re: your patch, does using an implicitly unwrapped optional work for both cases? e.g. [UnsafeMutablePointer<CInterop.PlatformChar>!]

I get the following error with Swift 5.9.2 on Android AArch64 if I add that to the type signature of libc_fts_open():

swift-nio/Sources/NIOFileSystem/Internal/System Calls/Syscalls.swift:407:14: error: using '!' is not allowed here; perhaps '?' was intended? _ path: [UnsafeMutablePointer<CInterop.PlatformChar>!], ^ ~ ? error: fatalError

@glbrntt, is that type what you had in mind, or simply unwrapping the arguments when they are passed in inside ftsOpen()?

I think that I approached it similarly to what was being suggested - rebinding the memory around the call site.

These fts_open() changes were no longer needed after swiftlang/swift#74829, so I removed them in #5010.

…ded in NDK 26

finagolfin · 2023-12-01T21:41:40Z

                // Filesizes are often 64bit even on 32bit systems
                let mapSize = min(length, Int(clamping: statbuf.st_size))
+              #if os(Android)
+                // Bionic mmap() now returns _Nonnull, so the force unwrap below isn't needed.


@parkera, is this the change you had in mind?

finagolfin · 2024-02-12T14:22:30Z

@parkera, any further feedback here?

compnerd · 2024-03-01T22:32:37Z

                return
            } else if errno == ENOTEMPTY {
+                #if os(Android)
+                let ps = UnsafeMutablePointer<UnsafeMutablePointer<Int8>>.allocate(capacity: 2)


Do we really need to duplicate this or can we get away with a rebinding of the memory?

No longer needed after your swiftlang/swift#74829, so removed in #5010.

compnerd · 2024-03-01T22:32:49Z

                ps.deallocate()

-                if stream != nil {
+                if let openStream = stream {


Why not simply if let stream?

Good point, removed in #5010, you can add it in a version of #4889 for 6.0.

compnerd · 2024-03-01T22:33:44Z

            do {
                guard fm.fileExists(atPath: _url.path) else { throw _NSErrorWithErrno(ENOENT, reading: true, url: url) }
                _stream = try FileManager.default._fileSystemRepresentation(withPath: _url.path) { fsRep in
+                    #if os(Android)


I think that I approached it similarly to what was being suggested - rebinding the memory around the call site.

compnerd · 2024-03-01T22:34:23Z

                return newItemURL.withUnsafeFileSystemRepresentation { (newItemFS) -> Int32? in
                    // This is an atomic operation in many OSes, but is not guaranteed to be atomic by the standard.
-                    if rename(newItemFS, originalFS) == 0 {
+                    if let newFS = newItemFS, let origFS = originalFS, rename(newFS, origFS) == 0 {


Why not simply force unwrap?

That's better, dropped this in #5010, you can do that in a version of #4889 for 6.0.

compnerd · 2024-03-01T22:34:47Z

    // getnameinfo uses size_t for its 4th and 6th arguments.
    private func getnameinfo(_ addr: UnsafePointer<sockaddr>?, _ addrlen: socklen_t, _ host: UnsafeMutablePointer<Int8>?, _ hostlen: socklen_t, _ serv: UnsafeMutablePointer<Int8>?, _ servlen: socklen_t, _ flags: Int32) -> Int32 {
-        return Glibc.getnameinfo(addr, addrlen, host, Int(hostlen), serv, Int(servlen), flags)
+        guard let saddr = addr else { return -1 }


I think that I prefer my approach of changing the signature of the private func.

Sorry about the delay, been meaning to get to this, hopefully this week.

Changing the signature would work too, dropped this in #5010, you can submit it in a version of #4889 for 6.0.

finagolfin · 2024-07-17T18:28:10Z

Now that swift-foundation was merged, this pull no longer applies cleanly to that rewrite. Instead, I have modified or removed portions of this pull based on the review comments here and applied it to the release/6.0 branch in #5010 instead, where swift-foundation will presumably not be merged also.

Once that's in 6.0, I will rework this pull for swift-foundation and make the necessary changes here also. My answers to the review comments above will now address what I changed for #5010.

finagolfin · 2024-08-11T02:46:19Z

Some of these are no longer needed after the re-core and other changes, others have been submitted again in #5024 and my pull for that, hyp#1.

finagolfin commented Dec 1, 2023

View reviewed changes

parkera reviewed Dec 1, 2023

View reviewed changes

Android: add better nullability checks for nullability annotations ad…

608fff3

…ded in NDK 26

finagolfin force-pushed the droid branch from cd1e20d to 608fff3 Compare December 1, 2023 21:40

finagolfin commented Dec 1, 2023

View reviewed changes

finagolfin mentioned this pull request Mar 1, 2024

Foundation: repair the build for Android API level 28+ #4889

Closed

compnerd reviewed Mar 1, 2024

View reviewed changes

finagolfin mentioned this pull request Jul 17, 2024

[6.0] Android: add better nullability checks for nullability annotations added in NDK 26 #5010

Closed

finagolfin closed this Aug 11, 2024

finagolfin deleted the droid branch August 11, 2024 02:50

marcprux mentioned this pull request Jan 10, 2025

Override nullability annotations in local NDK headers finagolfin/swift-android-sdk#200

Closed

Uh oh!

Conversation

finagolfin commented Dec 1, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parkera commented Dec 1, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

finagolfin commented Feb 12, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

finagolfin commented Jul 17, 2024

Uh oh!

finagolfin commented Aug 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants